Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Applayer plugin 5053 v3.19 #12364

Closed

Conversation

catenacyber
Copy link
Contributor

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5053

Describe changes:

  • app-layer plugins

#12363 without zabbix plugin in tree, and test fix due to splitting with #12307

Note that there is still #12307 to fix the limitation of probing parsers against 32 protocols (meaning any new app-layer like one in a plugin may be affected by this bug if it uses probing parsers for protocol detection)

Because some alprotos will remain static and defined as a constant,
such as ALPROTO_UNKNOWN=0, or ALPROTO_FAILED.

The regular already used protocols keep for now their static
identifier such as ALPROTO_SNMP, but this could be made more
dynamic in a later commit.

ALPROTO_FAILED was used in comparison and these needed to change to use
either ALPROTO_MAX or use standard function AppProtoIsValid
Ticket: 5053

The names are now dynamically registered at runtime.
The AppProto alproto enum identifiers are still static for now.

This is the final step before app-layer plugins.
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 57.53425% with 93 lines in your changes missing coverage. Please review.

Project coverage is 80.50%. Comparing base (494d7bf) to head (404395b).
Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12364      +/-   ##
==========================================
- Coverage   82.54%   80.50%   -2.05%     
==========================================
  Files         912      913       +1     
  Lines      258028   258152     +124     
==========================================
- Hits       212988   207818    -5170     
- Misses      45040    50334    +5294     
Flag Coverage Δ
fuzzcorpus 56.38% <50.72%> (-4.34%) ⬇️
livemode 19.40% <45.89%> (+<0.01%) ⬆️
pcap 44.41% <51.20%> (-0.02%) ⬇️
suricata-verify 63.15% <53.88%> (-0.04%) ⬇️
unittests 58.56% <47.03%> (+0.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24156

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting close I think, some comments inline

src/app-layer-protos.c Show resolved Hide resolved
src/app-layer-protos.h Show resolved Hide resolved
src/app-layer-protos.h Show resolved Hide resolved
@@ -1029,11 +1029,54 @@ void AppLayerListSupportedProtocols(void)
}

/***** Setup/General Registration *****/
static void AppLayerNamesSetup(void)
{
AppProtoRegisterProtoString(ALPROTO_UNKNOWN, "unknown");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these move into the parser registrations? E.g.

  /**
   * \brief Register the SMTP Protocol parser.
   */
  void RegisterSMTPParsers(void)
  {
      const char *proto_name = "smtp";

     AppProtoRegisterProtoString(ALPROTO_SMTP, proto_name);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find how and it is not obvious :

This does not work for rust parsers because they do not know the ALPROTO_SSH value and they find it using the string "ssh" passed to AppLayerRegisterProtocolDetection

Can this be studied as a next step, also making ALPROTO_SNMP dynamic ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

src/detect-engine-register.c Show resolved Hide resolved
src/detect-engine-register.c Show resolved Hide resolved
src/output.c Show resolved Hide resolved
src/output.c Show resolved Hide resolved
src/output.h Show resolved Hide resolved
@catenacyber
Copy link
Contributor Author

Next in #12372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants